Skip to content

CHORE: TEST PR Performance Report control path - #799

Draft
Gaurav Sharma (bewithgaurav) wants to merge 28 commits into
mainfrom
bewithgaurav/profiler-control-canary
Draft

CHORE: TEST PR Performance Report control path#799
Gaurav Sharma (bewithgaurav) wants to merge 28 commits into
mainfrom
bewithgaurav/profiler-control-canary

Conversation

@bewithgaurav

@bewithgaurav Gaurav Sharma (bewithgaurav) commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Work Item / Issue Reference

ADO Work Item: AB#44819

Summary

Disposable control canary for the merged PR Performance Report.

Jahnvi Thakkar (jahnvi480) and others added 7 commits May 7, 2026 12:09
@jahnvi480
PERF: Optimize fetch API performance with cached encodings, skip diag…
ae1ae31 
… on SUCCESS, __slots__ Row, and C++ Row construction - Cache decoding encoding strings in cursor __init__ to avoid 2 method calls + 2 dict.get() per fetch - Skip DDBCSQLGetAllDiagRecords on SQL_SUCCESS (ODBC spec: zero records on SUCCESS) - Replace param.encode('ascii') try/except with str.isascii() (C-level check) - Class-level _SQL_TO_C_TYPE lookup table (built once, shared across cursors) - Add __slots__ to Row class (eliminates per-instance __dict__, ~232 bytes/row savings) - Add Row._fast_create static method (bypasses __init__ for common case) - Add C++ construct_rows function (builds Row objects in tight C loop, avoiding Python loop overhead) - Zero-copy Row fast path when no converters/UUID processing needed Benchmark results (5-run average, richbench repeat=5 number=5): - Fetch one: -1.7x -> -1.4x (18% improvement) - Fetch many: -1.7x -> -1.3x (24% improvement) - 100 inserts: 4.9x -> 5.6x (14% faster) - SELECT: -1.1x -> -1.0x (on par with pyodbc) Profiler wall clock (50K rows): - fetchall: 176.7ms -> 158.1ms (11% faster) - fetchmany: 166.6ms -> 138.6ms (17% faster) No overlap with PR #549 (execute fast path) or PR #526 (simdutf).
@jahnvi480
Applying python linting changes
5b91325
@jahnvi480
FIX: Refresh fetch settings caches after connection changes
826a25d 
Preserve late output converter fallback behavior and UUID conversion while retaining the no-converter fast path. Add regression and cache operation-count coverage for all fetch APIs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@jahnvi480
Merge main into fetch performance branch
0f8841f 
Resolve fetch and Row conflicts while retaining current-main converter dispatch, lowercase column maps, profiling scopes, and CHAR decoding ctype. Extend fetch fast paths and regression coverage for the merged behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@jahnvi480
FIX: Address fetch diagnostics and native Row review findings
56c3d3b 
Validate the native row type argument, restore unconditional diagnostic retrieval, raise fetch errors before row processing, and retain fast paths when registered converters do not match. Add isolated crash and fetch contract regressions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@jahnvi480
FIX: Supply NULL indicators in scalar fetch paths
614cc36 
Preserve scalar SQL NULL values without suppressing fetch errors. Cover fixed-width types, LOB and bound fetch paths, literal NULL and OBJECT_ID results, and cursor recovery.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CHORE: Add profiler control canary
19c0e3f 
Create a disposable documentation-only change to validate the PR Performance Report end to end.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 18, 2026 06:22
@github-actions github-actions Bot added the pr-size: small Minimal code update label Sep 18, 2026
@github-actions

github-actions Bot commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Performance Report

⚠️ Performance regression detected

1 database task consistently slowed down across 2 measured environments.

0 IMPROVEMENTS 1 SLOWDOWN 2/2 ENVIRONMENTS

Signal fingerprint

Database task Unix / SQL Server 2022 Unix / SQL Server 2025
Repeated positional queries 1238.3% slower 1212.6% slower

The largest recorded phase increases for these tasks are shown below. Phase timings are supporting evidence, not root-cause proof.

Coverage: 2 of 2 environments completed. Advisory result; does not block merging.

Measured timings
Environment Database task Before After Change
Unix / SQL Server 2022 Repeated positional queries 42.046 ms 560.054 ms +1238.3%
Unix / SQL Server 2025 Repeated positional queries 42.358 ms 557.201 ms +1212.6%
Performance diagnostics

Phase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed.

Unix / SQL Server 2022

Repeated positional queries: py::execute::cpp_call +5.005 ms; ddbc::SQLExecute_wrap +4.836 ms; py::fetchone::diag_records +1.648 ms.

Unix / SQL Server 2025

Repeated positional queries: py::execute::cpp_call +3.582 ms; ddbc::SQLExecute_wrap +3.490 ms; py::fetchone::diag_records +1.198 ms.

All database tasks and timings

Unix / SQL Server 2022

Database task Before After Paired change Result
Connection opening 10.256 ms 10.198 ms +0.2% no signal
SELECT queries 1.166 ms 1.150 ms -0.8% no signal
Row insertion 34.350 ms 33.994 ms -0.8% no signal
Executemany inserts 157.066 ms 156.217 ms -0.0% no signal
Fetch-all queries 170.462 ms 173.293 ms +1.1% no signal
Row-by-row fetching 62.062 ms 60.452 ms -2.1% no signal
Batched row fetching 165.938 ms 168.008 ms +0.7% no signal
Transaction commit and rollback 114.563 ms 113.984 ms -2.2% no signal
Arrow row fetching 92.861 ms 96.216 ms +3.3% no signal
100,000-row insertion 441.808 ms 454.252 ms +3.7% no signal
Row fetching in batches of 100 221.951 ms 222.330 ms +1.3% no signal
Row fetching in batches of 10,000 182.198 ms 191.535 ms +2.1% no signal
Repeated positional queries 42.046 ms 560.054 ms +1238.3% consistent slowdown
Repeated named-parameter queries 45.254 ms 44.990 ms +0.1% no signal
Legacy 100,000-row insertion 350.005 ms 346.173 ms -1.1% no signal
Insertion with explicit input sizes 2368.445 ms 2348.556 ms +0.7% no signal
Joined aggregation queries 176.524 ms 176.090 ms -0.1% no signal
Large joined-result fetching 208.796 ms 212.407 ms +1.4% no signal
1.2-million-row fetching 4949.515 ms 4992.226 ms +0.9% no signal
Common table expression queries 5.383 ms 5.396 ms +0.1% no signal

Unix / SQL Server 2025

Database task Before After Paired change Result
Connection opening 96.615 ms 96.817 ms +0.2% no signal
SELECT queries 1.179 ms 1.179 ms +0.2% no signal
Row insertion 34.177 ms 34.254 ms +1.5% no signal
Executemany inserts 151.787 ms 149.185 ms -0.0% no signal
Fetch-all queries 172.304 ms 170.996 ms -0.7% no signal
Row-by-row fetching 60.726 ms 60.618 ms -1.2% no signal
Batched row fetching 165.770 ms 165.143 ms -1.2% no signal
Transaction commit and rollback 114.270 ms 114.966 ms +0.4% no signal
Arrow row fetching 94.245 ms 94.386 ms +1.4% no signal
100,000-row insertion 448.389 ms 440.919 ms -0.0% no signal
Row fetching in batches of 100 222.935 ms 223.007 ms +0.5% no signal
Row fetching in batches of 10,000 176.679 ms 189.779 ms +9.3% no signal
Repeated positional queries 42.358 ms 557.201 ms +1212.6% consistent slowdown
Repeated named-parameter queries 44.831 ms 44.670 ms -0.1% no signal
Legacy 100,000-row insertion 351.679 ms 346.373 ms -1.4% no signal
Insertion with explicit input sizes 2293.433 ms 2352.178 ms +0.6% no signal
Joined aggregation queries 160.220 ms 161.338 ms +0.5% no signal
Large joined-result fetching 203.989 ms 204.435 ms -0.1% no signal
1.2-million-row fetching 5034.947 ms 5082.198 ms +0.4% no signal
Common table expression queries 5.210 ms 5.260 ms -0.4% no signal
Build and measurement details

ADO build 177104

PR head: c9658cfaa866570fb8c18258c47738576a3499f1
Base: f3e34e66bca6d6f45d7a2cbea070c40ebe4ba194
Measured merge: 2413a5a6d5b0ab1d2fba7d9604742cfe8a288332

  • Unix / SQL Server 2022: Python 3.12.3, x86_64, SQL 16.0.4295.3; 5 paired comparisons and 1 warmup.
  • Unix / SQL Server 2025: Python 3.12.3, x86_64, SQL 17.0.5005.3; 5 paired comparisons and 1 warmup.

A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent.

The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes.

Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency.

Raw samples and logs are attached to the ADO run as profiler-* artifacts.

Copilot AI reviewed Sep 18, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved issues; the change is limited to a non-rendered README marker.

Pull request overview

Adds an invisible README marker to validate the profiler report control path without changing product code.

Changes:

  • Adds an HTML comment canary to README.md.
File summaries
File Description
README.md Adds the invisible profiler control marker.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

93%

🎯 Overall Coverage

84%

📈 Total Lines Covered: 8767 out of 10412
📁 Project: mssql-python

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/connection.py (100%)
  • mssql_python/cursor.py (100%)
  • mssql_python/pybind/ddbc_bindings.cpp (83.3%): Missing lines 3634-3636,3650-3652,3655
  • mssql_python/row.py (100%)

Summary

  • Total: 103 lines
  • Missing: 7 lines
  • Coverage: 93%

mssql_python/pybind/ddbc_bindings.cpp

Lines 3630-3640

  3630             case SQL_SMALLINT: {
  3631                 SQLSMALLINT smallIntValue;
  3632                 SQLLEN indicator = 0;
  3633                 ret = SQLGetData_ptr(hStmt, i, SQL_C_SHORT, &smallIntValue, 0, &indicator);
! 3634                 if (SQL_SUCCEEDED(ret) && indicator == SQL_NULL_DATA) {
! 3635                     row.append(py::none());
! 3636                     break;
  3637                 }
  3638                 if (SQL_SUCCEEDED(ret)) {
  3639                     row.append(static_cast<int>(smallIntValue));
  3640                 } else {

Lines 3646-3659

  3646                 break;
  3647             }
  3648             case SQL_REAL: {
  3649                 SQLREAL realValue;
! 3650                 SQLLEN indicator = 0;
! 3651                 ret = SQLGetData_ptr(hStmt, i, SQL_C_FLOAT, &realValue, 0, &indicator);
! 3652                 if (SQL_SUCCEEDED(ret) && indicator == SQL_NULL_DATA) {
  3653                     row.append(py::none());
  3654                     break;
! 3655                 }
  3656                 if (SQL_SUCCEEDED(ret)) {
  3657                     row.append(realValue);
  3658                 } else {
  3659                     LOG("SQLGetData: Error retrieving SQL_REAL for column %d - "

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand) 
mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 78.5%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.py_type_cache.hpp: 91.6%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@bewithgaurav Gaurav Sharma (bewithgaurav) changed the title CHORE: Verify profiler report control path TEST: Verify PR Performance Report control path Sep 18, 2026
@bewithgaurav Gaurav Sharma (bewithgaurav) changed the title TEST: Verify PR Performance Report control path CHORE: Verify PR Performance Report control path Sep 18, 2026
@bewithgaurav Gaurav Sharma (bewithgaurav) changed the title CHORE: Verify PR Performance Report control path CHORE: TEST PR Performance Report control path Sep 18, 2026
Copilot AI review requested due to automatic review settings September 18, 2026 10:42
Copilot AI reviewed Sep 18, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved issues were identified.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@jahnvi480
Merge branch 'main' into jahnvi/perf-fetch-optimization
ed7f480
@jahnvi480
Merge main into fetch performance branch
7b76159 
Merge main 2a86fc1 while preserving fetch cache and NULL fixes. Share canonical Row mapping snapshots through Python and native fast constructors, including legacy constructor defaults. Retain main's pooled transaction cleanup changes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@jahnvi480
FIX: Keep native Row attribute names local to each call
532efec 
Replace process-static Python attribute names with scoped owning handles outside the row loop. Cover repeated batch construction and reference cleanup when slot assignment fails.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bewithgaurav
Merge branch 'main' into jahnvi/perf-fetch-optimization
a42a7ca
@bewithgaurav
Merge branch 'main' into bewithgaurav/profiler-control-canary
e883f2a
Copilot AI review requested due to automatic review settings September 21, 2026 08:18
Copilot AI reviewed Sep 21, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The scope is not README-only, report coverage is incomplete, and the Row changes may break public API compatibility.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity · 1 Low severity

Open (3)

Comment thread eng/profiler_benchmarks/report.py
Comment on lines +436 to +440
f"<kbd>{improvement_tasks} IMPROVEMENT"
f"{'S' if improvement_tasks != 1 else ''}</kbd> "
f"<kbd>{regression_tasks} SLOWDOWN"
f"{'S' if regression_tasks != 1 else ''}</kbd> "
f"<kbd>{len(completed)}/{len(LEGS)} ENVIRONMENTS</kbd>",
Copilot AI review requested due to automatic review settings September 21, 2026 15:22
@bewithgaurav
FIX: Run only profiler-producing canary legs
6dddc9f 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI reviewed Sep 21, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Address the public Row API break, native reference leak, and mismatches between the stated canary scope and CI coverage.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity · 1 Low severity

Open (3)

Copilot AI review requested due to automatic review settings September 21, 2026 15:28
Copilot AI reviewed Sep 21, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical CI coverage changes and unresolved fetch-performance and Row compatibility regressions must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity · 1 Low severity

Open (4)

Comment thread eng/pipelines/pr-validation-pipeline.yml
jobs:
- job: CodeQLAnalysis
displayName: 'CodeQL Security Analysis'
condition: false
@bewithgaurav
STYLE: Refine performance signal summary
e1141c5 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 21, 2026 18:38
Copilot AI reviewed Sep 21, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Broad CI coverage is disabled, and the implementation scope conflicts with the README-only canary description.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity · 1 Low severity

Open (4)

@bewithgaurav
TEST: Exercise profiler regression reporting
e72305b 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 04:01
Copilot AI reviewed Sep 22, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved production behavior, validation coverage, reporting, and scope issues must be addressed before approval.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity · 1 Low severity

Open (4)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Disabled jobs remove validation across the supported platform matrix

eng/​pipelines/​pr-validation-pipeline.yml:55

Setting the whole Windows job to condition: false removes Windows functional validation; the same change also disables macOS, ARM64, RHEL, Alpine, and coverage jobs in this pipeline. This leaves only the two Ubuntu profiler legs active, so the main PR validation no longer covers the supported matrix.

@bewithgaurav
TEST: Isolate profiler slowdown canary
931b66a 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 04:56
@github-actions github-actions Bot added pr-size: medium Moderate update size and removed pr-size: large Substantial code update labels Sep 22, 2026
Copilot AI reviewed Sep 22, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Remove the runtime delay and restore the disabled security analysis gate.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (2)

@bewithgaurav
FIX: Run same-repo profiler publisher from PR code
c05ec03 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 05:06
Copilot AI reviewed Sep 22, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical workflow credential exposure, disabled validation coverage, and production-path latency remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 High severity · 1 Medium severity

Open (4)

Comment thread .github/workflows/pr-profiler-report.yml
Comment on lines +22 to +26
if: >-
(github.event_name == 'pull_request' &&
github.event.pull_request.head.repo.full_name == github.repository) ||
(github.event_name == 'pull_request_target' &&
github.event.pull_request.head.repo.full_name != github.repository)
Comment thread eng/pipelines/pr-validation-pipeline.yml

- job: pytestonwindows
displayName: 'Windows x64'
condition: false
@bewithgaurav
STYLE: Remove performance spread column
c9658cf 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 05:41
Copilot AI reviewed Sep 22, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical workflow and security issues, CI coverage loss, an incorrect partial-results verdict, and a runtime performance regression remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 High severity · 1 Medium severity · 1 Low severity

Open (6)

Comment thread .github/workflows/pr-profiler-report.yml
pull_request:
branches: [main]
types: [opened, synchronize, reopened, ready_for_review]
pull_request_target:
Comment thread eng/profiler_benchmarks/README.md
Comment on lines +33 to +35
remain covered by functional CI. Same-repository PRs run their formatter directly;
fork PRs retain the trusted-base publisher. Both select the exact PR-head ADO build
and validate bounded artifacts as data. Publication begins as soon as both profiler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

pr-size: medium Moderate update size

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bewithgaurav @jahnvi480